Skip to content

Add DenyPSALabel admission plugin#10950

Closed
galal-hussein wants to merge 6 commits into
k3s-io:mainfrom
galal-hussein:psa
Closed

Add DenyPSALabel admission plugin#10950
galal-hussein wants to merge 6 commits into
k3s-io:mainfrom
galal-hussein:psa

Conversation

@galal-hussein
Copy link
Copy Markdown
Contributor

@galal-hussein galal-hussein commented Sep 25, 2024

Proposed Changes

The PR adds a new admission plugin to the apiserver, this has become available by the recent patch k3s-io/kubernetes@94d3e60 which allows the users to register a new plugin dynamically when starting the API server.

The plugin "DenyPSALabel" will deny the overriding of the default PSA security configuration passed to the API server, by default this plugin will not start nor any behavior will be different unless the user passes the following arguments to k3s:

k3s server --deny-psa-label --kube-apiserver-arg="enable-admission-plugin=DenyPSALabel"

The first flag will register the plugin to the apiserver and the second flag will enable it in the runtime

Types of Changes

Testing

  • After starting the feature user shouldnt be able to add the following label to the namespace, for example:
kubectl label --dry-run=server --overwrite ns --all \
    pod-security.kubernetes.io/enforce=baseline

should result in an error.

Linked Issues

User-Facing Change


Further Comments

Signed-off-by: galal-hussein <hussein.galal.ahmed.11@gmail.com>
@galal-hussein galal-hussein requested a review from a team as a code owner September 25, 2024 21:01
Comment thread pkg/admission/denypsalabel/admission.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 25, 2024

Codecov Report

❌ Patch coverage is 3.22581% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.11%. Comparing base (ed14f7f) to head (16242c6).
⚠️ Report is 676 commits behind head on main.

Files with missing lines Patch % Lines
pkg/admission/denypsalabel/admission.go 0.00% 26 Missing ⚠️
pkg/daemons/control/server.go 0.00% 3 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (ed14f7f) and HEAD (16242c6). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (ed14f7f) HEAD (16242c6)
unittests 1 0
inttests 10 9
e2etests 7 6
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10950      +/-   ##
==========================================
- Coverage   49.64%   40.11%   -9.54%     
==========================================
  Files         178      162      -16     
  Lines       14801    14354     -447     
==========================================
- Hits         7348     5758    -1590     
- Misses       6105     7405    +1300     
+ Partials     1348     1191     -157     
Flag Coverage Δ
e2etests 36.42% <3.22%> (-9.55%) ⬇️
inttests 19.63% <3.22%> (-0.09%) ⬇️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: galal-hussein <hussein.galal.ahmed.11@gmail.com>
Comment thread pkg/admission/denypsalabel/admission.go Outdated
Comment thread pkg/admission/denypsalabel/admission.go Outdated
Comment thread pkg/admission/denypsalabel/admission.go Outdated
Comment thread pkg/cli/cmds/server.go Outdated
Signed-off-by: galal-hussein <hussein.galal.ahmed.11@gmail.com>
brandond
brandond previously approved these changes Sep 26, 2024
vitorsavian
vitorsavian previously approved these changes Sep 30, 2024
Copy link
Copy Markdown
Member

@dereknola dereknola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this is merged, @galal-hussein can you fill out this PR more? There isn't any linked issues and there is no testing for a new feature. You should be able to add a new testlet to the startup integration test https://github.com/k3s-io/k3s/blob/master/tests/integration/startup/startup_int_test.go

@galal-hussein
Copy link
Copy Markdown
Contributor Author

@dereknola sure will fix that

Signed-off-by: galal-hussein <hussein.galal.ahmed.11@gmail.com>
@galal-hussein galal-hussein dismissed stale reviews from vitorsavian and brandond via 81261af October 2, 2024 20:50
dereknola
dereknola previously approved these changes Oct 7, 2024
Signed-off-by: galal-hussein <hussein.galal.ahmed.11@gmail.com>
Signed-off-by: galal-hussein <hussein.galal.ahmed.11@gmail.com>
It("change label of namespace", func() {
res, err := testutil.K3sCmd("kubectl label --dry-run=server --overwrite ns --all pod-security.kubernetes.io/enforce=baseline")
Expect(err).To(HaveOccurred())
Expect(res).To(ContainSubstring("denying use of PSA label on namespace"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this test will fail as-is, since this is not the string that is output any longer.

Suggested change
Expect(res).To(ContainSubstring("denying use of PSA label on namespace"))
Expect(res).To(ContainSubstring("Use of label with pod-security.kubernetes.io/ prefix on Namespace is denied by admission control"))

@brandond
Copy link
Copy Markdown
Member

@galal-hussein are we still going to add this, or can we close?

@galal-hussein
Copy link
Copy Markdown
Contributor Author

This is still needed so that PSA feature can work in k3k virtual mode

@brandond
Copy link
Copy Markdown
Member

brandond commented Mar 7, 2026

@galal-hussein do you still need this?

@galal-hussein
Copy link
Copy Markdown
Contributor Author

@brandond No not really, I will close the PR, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants